Skip to content

feat(security/tools): P0-2 实现 Workspace Sandbox（含 symlink / traversal 防护）#116

Merged
minorcell merged 10 commits into
1024XEngineer:mainfrom
Cai-Tang-www:main
Apr 2, 2026
Merged

feat(security/tools): P0-2 实现 Workspace Sandbox（含 symlink / traversal 防护）#116
minorcell merged 10 commits into
1024XEngineer:mainfrom
Cai-Tang-www:main

Conversation

@Cai-Tang-www

@Cai-Tang-www Cai-Tang-www commented Apr 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

背景

承接 #98 和 #106，本次实现 Workspace Sandbox 的第一版真实能力。
目标是在不破坏主链路的前提下，把工具路径边界从 noop 升级为可执行、可测试的安全检查。

执行链保持为：

Runtime -> ToolManager -> PermissionEngine -> WorkspaceSandbox -> Executor

本次改动

1. 新增真实 Workspace Sandbox

  • 新增 internal/security/workspace.go
  • 支持统一路径边界校验：
    • path traversal 防护（..）
    • workspace 外绝对路径防护
    • symlink escape 防护（含父目录 symlink 场景）
  • 对 read/write/bash 类 action 生效
  • 非路径型 action 不误伤（如 webfetch）

2. 扩展 Action Payload 以支持 sandbox 判定

  • 更新 internal/security/types.go
  • 新增 SandboxTargetType、SandboxTarget 字段
  • 为 sandbox 提供独立目标信息，避免与权限展示字段耦合

3. 更新工具 action 映射

  • 更新 internal/tools/permission_mapper.go
  • 为以下工具补充 sandbox 目标映射：
    • filesystem_read_file
    • filesystem_write_file
    • filesystem_edit
    • filesystem_grep
    • filesystem_glob
    • bash（读取 arguments 中的 workdir）

4. 接入执行链与错误元数据

  • 更新 internal/tools/manager.go
  • sandbox 拒绝时统一返回 workspace sandbox rejected action
  • metadata 增加 sandbox 目标信息，便于排障与后续 UI 展示

5. 装配层启用真实 sandbox

  • 更新 internal/app/bootstrap.go
  • buildToolManager 不再使用 noop sandbox，改为 security.NewWorkspaceSandbox()

测试

1. 新增 internal/security/workspace_test.go

table-driven 覆盖：

  • workspace 内正常路径
  • traversal 越界
  • workspace 外绝对路径
  • symlink 文件越界
  • symlink 父目录越界
  • bash workdir 限制
  • 非路径型 action 放行

2. 更新 internal/tools/manager_test.go

  • 补充 sandbox 目标映射断言
  • 补充 manager + 真实 sandbox 的执行链阻断测试

3. 更新 internal/app/bootstrap_test.go

  • 验证 bootstrap 接入真实 sandbox 后可拒绝越界 workdir

兼容性与风险

  • 主链路不变，改动集中在 tools/security/app 装配层
  • 文件工具原有路径校验暂保留，形成“sandbox 前置 + 工具兜底”双层保护
  • 在部分环境（尤其 Windows 权限限制）symlink 测试会按预期 skip，不影响主流程

验证

已本地通过：

go test ./...

Refs

@Cai-Tang-www Cai-Tang-www changed the title feat: 实现 Workspace Sandbox 第一版真实能力 feat(security/tools): P0-2 实现 Workspace Sandbox（含 symlink / traversal 防护） Apr 2, 2026
@codecov

codecov Bot commented Apr 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.96703% with 31 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/security/workspace.go 81.21% 17 Missing and 14 partials ⚠️

📢 Thoughts on this report? Let us know!

@minorcell

minorcell commented Apr 2, 2026

Copy link
Copy Markdown
Member

/review

minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/workspace.go
}

return nil
}

@minorcell minorcell Apr 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL SECURITY: TOCTOU Race Condition Vulnerability

This function has a Time-of-Check-Time-of-Use (TOCTOU) vulnerability. The validation happens here, but the actual file operation occurs later in the tool executor. An attacker can:

  1. Pass validation with a safe symlink pointing inside workspace
  2. Between validation and file access, atomically replace the symlink to point outside workspace
  3. Tool executor accesses the malicious path (sandbox bypassed)

Attack scenario:

# During validation: workspace/link -> workspace/safe.txt (PASSES)
# Attacker replaces: workspace/link -> /etc/passwd
# Tool reads workspace/link (BYPASSED)

Recommendations:

  1. Open file descriptor during validation and pass to executor (requires executor refactor)
  2. Use O_NOFOLLOW flag on Linux or FILE_FLAG_OPEN_REPARSE_POINT on Windows
  3. At minimum, document this limitation and consider adding re-validation in critical paths
  4. Add mutex/locking around validation + execution for high-security scenarios

Impact: Complete sandbox bypass, arbitrary file read/write outside workspace.

@Cai-Tang-www Cai-Tang-www Apr 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这不是 workspace sandbox 这一层单独能解决的，得进执行器层，走 descriptor/no-follow/原子访问
我会留到后面修改

minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/workspace.go Outdated
minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/workspace.go Outdated
minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/workspace.go Outdated
minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/workspace.go
minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/workspace.go
minorcell
minorcell reviewed Apr 2, 2026
View reviewed changes
Comment thread internal/security/types.go
@minorcell

minorcell commented Apr 2, 2026

Copy link
Copy Markdown
Member

Comprehensive Code Review Summary

Conducted in-depth review across code quality, performance, security, and documentation. The implementation is well-structured with solid test coverage, but critical security vulnerabilities require attention before merge.

🚨 Critical Issues (Block Merge)

  • TOCTOU race condition in symlink validation - sandbox can be bypassed
  • Non-existent workspace root allows symlink bypass
  • Windows UNC paths not properly validated

⚡ Performance Concerns

  • Workspace root resolved on every call (~100 unnecessary syscalls/session)
  • Path-by-path symlink walking in nested directories

📝 Code Quality

  • Strong test coverage and separation of concerns
  • Missing documentation on security-critical functions
  • Some interface placement questions

Recommendation: Address security issues before merging. See inline comments for details.

@minorcell minorcell merged commit 03ef5a4 into 1024XEngineer:main Apr 2, 2026
2 checks passed
@Cai-Tang-www Cai-Tang-www mentioned this pull request Apr 4, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@minorcell minorcell minorcell left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Cai-Tang-www @minorcell